Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: mixture density computation in multiphase poromechanics solvers #3342

Merged
merged 6 commits into from
Sep 19, 2024

Conversation

castelletto1
Copy link
Contributor

This PR modifies the computation of the mixture (solid + fluid) density favoring use of primary variables over secondary variables.

@castelletto1 castelletto1 added the type: cleanup / refactor Non-functional change (NFC) label Sep 9, 2024
@castelletto1 castelletto1 self-assigned this Sep 9, 2024
@castelletto1
Copy link
Contributor Author

We should decide whether we want to refer to the expression:

$$\rho=(1-\phi)\rho_{\text{solid}}+\phi\sum_{c}\rho_c=(1-\phi)\rho_{\text{solid}}+\phi\sum_{c}\sum_{\ell=1}^{np}x_{c\ell}\rho_{\ell}s_{\ell}=(1-\phi)\rho_{\text{solid}}+\phi\sum_{\ell=1}^{np}\rho_{\ell}s_{\ell}$$

as mixture density or bulk density, and be consistent everywhere.

@castelletto1 castelletto1 added ci: run integrated tests Allows to run the integrated tests in GEOS CI ci: run CUDA builds Allows to triggers (costly) CUDA jobs flag: ready for review labels Sep 9, 2024
Copy link
Contributor

@paveltomin paveltomin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 56.32%. Comparing base (7130462) to head (122ab3f).
Report is 85 commits behind head on develop.

Files with missing lines Patch % Lines
...s/poromechanicsKernels/MultiphasePoromechanics.hpp 0.00% 10 Missing ⚠️
...omechanicsKernels/MultiphasePoromechanics_impl.hpp 0.00% 7 Missing ⚠️
...csSolvers/multiphysics/MultiphasePoromechanics.cpp 0.00% 1 Missing ⚠️
...icsKernels/ThermalMultiphasePoromechanics_impl.hpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3342      +/-   ##
===========================================
+ Coverage    56.30%   56.32%   +0.01%     
===========================================
  Files         1065     1065              
  Lines        90227    90210      -17     
===========================================
+ Hits         50805    50807       +2     
+ Misses       39422    39403      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


for( integer ip = 0; ip < m_numPhases; ++ip )
{
dTotalMassDensity_dTemperature += dPhaseVolFrac( ip, Deriv::dT ) * phaseMassDensity( ip )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this means that these terms cancel out and we should end up with a value that is numerically close to zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we highlight in the body force term the dependencies on the primary variables we have:

$$\rho \mathbf{g}=(1-\phi(\epsilon_v(\mathbf{u}),p,T))\rho_{\text{solid}}(p,T)\mathbf{g}+\phi(\epsilon_v(\mathbf{u}),p,T)\sum_{c}\rho_c\mathbf{g}$$

We have a direct dependency on temperature exclusively on porosity at the moment -- eventually on the solid phase density $\rho_s$ (not considered at the moment in the code). I agree that one can probably show that looping over phases produces dTotalMassDensity_dTemperature = 0.

@CusiniM
Copy link
Collaborator

CusiniM commented Sep 10, 2024

We should decide whether we want to refer to the expression:

ρ = ( 1 − ϕ ) ρ solid + ϕ ∑ c ρ c = ( 1 − ϕ ) ρ solid + ϕ ∑ c ∑ ℓ = 1 n p x c ℓ ρ ℓ s ℓ = ( 1 − ϕ ) ρ solid + ϕ ∑ ℓ = 1 n p ρ ℓ s ℓ

as mixture density or bulk density, and be consistent everywhere.

can we call this totalDensity instead of bulk density ? ? It's certainly not the mixture density.

@CusiniM
Copy link
Collaborator

CusiniM commented Sep 10, 2024

We should decide whether we want to refer to the expression:
ρ = ( 1 − ϕ ) ρ solid + ϕ ∑ c ρ c = ( 1 − ϕ ) ρ solid + ϕ ∑ c ∑ ℓ = 1 n p x c ℓ ρ ℓ s ℓ = ( 1 − ϕ ) ρ solid + ϕ ∑ ℓ = 1 n p ρ ℓ s ℓ
as mixture density or bulk density, and be consistent everywhere.

can we call this totalDensity instead of bulk density ? ? It's certainly not the mixture density.

Looking at the code, IMO, what we call totalMassDensity should be called mixtureDensity and the totalDensity should include the solid part.

@castelletto1
Copy link
Contributor Author

We should decide whether we want to refer to the expression:
ρ = ( 1 − ϕ ) ρ solid + ϕ ∑ c ρ c = ( 1 − ϕ ) ρ solid + ϕ ∑ c ∑ ℓ = 1 n p x c ℓ ρ ℓ s ℓ = ( 1 − ϕ ) ρ solid + ϕ ∑ ℓ = 1 n p ρ ℓ s ℓ
as mixture density or bulk density, and be consistent everywhere.

can we call this totalDensity instead of bulk density ? ? It's certainly not the mixture density.

Looking at the code, IMO, what we call totalMassDensity should be called mixtureDensity and the totalDensity should include the solid part.

I agree it is confusing. Often, in the poromechanics literature authors refer to mixture including the solid phase, for example: "... a three-phase mixture composed of a solid matrix whose voids are continuous and filled
with water and air ..."
[Borja, CMAME (2004)].

In my opinion, we should agree on consistent naming for:

  • solid phase(s)
  • fluid phases and their mixture
  • solid/fluid combined

Also the notation used in the documentation should be updated accordingly.

@castelletto1 castelletto1 added the flag: no rebaseline Does not require rebaseline label Sep 10, 2024
@CusiniM
Copy link
Collaborator

CusiniM commented Sep 11, 2024

We should decide whether we want to refer to the expression:
ρ = ( 1 − ϕ ) ρ solid + ϕ ∑ c ρ c = ( 1 − ϕ ) ρ solid + ϕ ∑ c ∑ ℓ = 1 n p x c ℓ ρ ℓ s ℓ = ( 1 − ϕ ) ρ solid + ϕ ∑ ℓ = 1 n p ρ ℓ s ℓ
as mixture density or bulk density, and be consistent everywhere.

can we call this totalDensity instead of bulk density ? ? It's certainly not the mixture density.

Looking at the code, IMO, what we call totalMassDensity should be called mixtureDensity and the totalDensity should include the solid part.

I agree it is confusing. Often, in the poromechanics literature authors refer to mixture including the solid phase, for example: "... a three-phase mixture composed of a solid matrix whose voids are continuous and filled with water and air ..." [Borja, CMAME (2004)].

In my opinion, we should agree on consistent naming for:

* solid phase(s)

* fluid phases and their mixture

* solid/fluid combined

Also the notation used in the documentation should be updated accordingly.

I mean, I think the definition of mixture is quite broad so technically one could see the solid + fluids as a mixture. However, since we are often interested in the properties of just the fluid mixture I think that it is more straightforward to call:

  • mixture or fluidMixture the mixture of the fluid phases and so refer to the mixtureDensity or the fluidMixtureDensity for that one.
  • totalDensity the density of the fluidMixture plus the solid. And I d do the same thing with energy, enthalpy etc.
    Mainly coz it's weird to have something called totalDensity that does not include all phases present.

@paveltomin
Copy link
Contributor

  • totalDensity the density of the fluidMixture plus the solid. And I d do the same thing with energy, enthalpy etc.
    Mainly coz it's weird to have something called totalDensity that does not include all phases present.

i would say totalDensity is kind of reserved for fluid total density

@CusiniM
Copy link
Collaborator

CusiniM commented Sep 11, 2024

  • totalDensity the density of the fluidMixture plus the solid. And I d do the same thing with energy, enthalpy etc.
    Mainly coz it's weird to have something called totalDensity that does not include all phases present.

i would say totalDensity is kind of reserved for fluid total density

yeah, I actually don't like the name totalDensity for the fluid mixture one. But at the end of the day as long as we are consistent, it's fine.

@CusiniM CusiniM merged commit 7ff2349 into develop Sep 19, 2024
24 of 25 checks passed
@CusiniM CusiniM deleted the feature/castelletto1/poromechanicsKernels branch September 19, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: no rebaseline Does not require rebaseline flag: ready for review type: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants